Add Arrow fixed-size-list ingress - #23583
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Arrow interop layer detects fixed-size-list schemas, converts them to cuDF ChangesFixed-size-list Arrow interoperability
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/interop/from_arrow_host.cu (1)
608-717: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAdd boundary and multi-block fixed-size-list cases.
The tests cover empty, null, and sliced input. They do not cover boundary or multi-block row counts. Add fixed-size-list cases that cross the relevant execution-size boundary.
As per coding guidelines, “Tests must cover empty inputs, nulls, sliced columns, boundary and multi-block sizes.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/interop/from_arrow_host.cu` around lines 608 - 717, Add fixed-size-list test cases covering row counts at the relevant execution-size boundary and counts large enough to span multiple blocks. Extend the existing tests for empty, null, and sliced inputs, using the from_arrow_column conversion path and validating both conversion success and resulting values.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/interop/from_arrow_host.cu`:
- Around line 297-317: Update get_fixed_size_list_offsets to validate
input->offset and input->length are non-negative before arithmetic, handle width
== 0 explicitly, and verify input->length + 1 fits cudf::size_type. Check each
multiplication for int64_t overflow before computing offset and length, then
retain the existing child-element limit validation and only cast after all
bounds checks pass.
In `@cpp/tests/interop/from_arrow_device_test.cpp`:
- Around line 310-388: Expand the fixed-size-list coverage in
FixedSizeListColumn and FixedSizeListColumnSliced in
cpp/tests/interop/from_arrow_device_test.cpp:310-388 to include empty inputs,
nullable list rows, boundary sizes, and multi-block sizes while preserving
existing full and sliced cases. Extend the corresponding fixed-size-list tests
in cpp/tests/interop/from_arrow_host_test.cpp:608-717 with boundary-size and
multi-block-size cases; no empty or nullable cases are requested there.
In `@cpp/tests/interop/from_arrow_host_test.cpp`:
- Around line 608-717: Extend the fixed-size-list tests around
FixedSizeListColumn with a case at the relevant conversion boundary and another
whose row count exceeds one execution block. Cover both valid and nullable
inputs as appropriate, and assert the resulting offsets, child values, and null
masks (including nonempty null rows where applicable) rather than only table
equivalence.
In `@cpp/tests/interop/from_arrow_stream_test.cpp`:
- Around line 174-209: Expand the fixed-size-list coverage in
cpp/tests/interop/from_arrow_stream_test.cpp at lines 174-209 by adding
null-row, nonzero-offset sliced, boundary-size, and multi-block-size stream
cases alongside FixedSizeListEmptyTest and FixedSizeListChunkedTest. Expand the
related coverage in python/pylibcudf/tests/test_table.py at lines 110-120 with
empty, all-null, and single-row arrays, plus a mixed-type table containing a
fixed-size-list column; no other sites require changes.
---
Outside diff comments:
In `@cpp/src/interop/from_arrow_host.cu`:
- Around line 608-717: Add fixed-size-list test cases covering row counts at the
relevant execution-size boundary and counts large enough to span multiple
blocks. Extend the existing tests for empty, null, and sliced inputs, using the
from_arrow_column conversion path and validating both conversion success and
resulting values.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a1a9d9ea-0721-4ce2-b20b-09819fa50260
📒 Files selected for processing (11)
cpp/src/interop/arrow_data_structures.cppcpp/src/interop/arrow_utilities.cppcpp/src/interop/arrow_utilities.hppcpp/src/interop/from_arrow_device.cucpp/src/interop/from_arrow_host.cucpp/src/interop/from_arrow_host.hppcpp/tests/interop/arrow_data_structures_test.cppcpp/tests/interop/from_arrow_device_test.cppcpp/tests/interop/from_arrow_host_test.cppcpp/tests/interop/from_arrow_stream_test.cpppython/pylibcudf/tests/test_table.py
|
Addressed in fdf87a7: added overflow-safe bounds validation, including negative metadata, zero width, multiplication overflow, and short-child checks |
|
Ready for maintainer vetting. Could someone add feature request and non-breaking labels and approve NVIDIA runner workflows? |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/interop/arrow_utilities.cpp`:
- Around line 111-121: In the fixed-size-list bounds validation before
constructing the returned range, add a check that row_end is at most max_size /
width after the existing int64_t overflow check. Ensure this rejects absolute
child ranges exceeding cudf::size_type before the child_length validation and
return in the surrounding conversion function.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ae361867-000a-429d-8c1b-9343c94183eb
📒 Files selected for processing (8)
cpp/src/interop/arrow_utilities.cppcpp/src/interop/arrow_utilities.hppcpp/src/interop/from_arrow_device.cucpp/src/interop/from_arrow_host.cucpp/tests/interop/from_arrow_device_test.cppcpp/tests/interop/from_arrow_host_test.cppcpp/tests/interop/from_arrow_stream_test.cpppython/pylibcudf/tests/test_table.py
🚧 Files skipped from review as they are similar to previous changes (3)
- python/pylibcudf/tests/test_table.py
- cpp/src/interop/from_arrow_device.cu
- cpp/src/interop/from_arrow_host.cu
fdf87a7 to
625a3af
Compare
625a3af to
8599f6f
Compare
|
I have pending fix to promote, but PR push is blocked likely due to RAPIDS → NVIDIA CUDA-X transition. |
Closes #23545
Validation on RTX A6000, CUDA 13.3: